Skip to content

feat: support explicit raw secret values - #2873

Open
Mikhail Shirkov (shirkevich) wants to merge 5 commits into
codex/secrets-safe-maskingfrom
codex/secrets-raw-values
Open

feat: support explicit raw secret values#2873
Mikhail Shirkov (shirkevich) wants to merge 5 commits into
codex/secrets-safe-maskingfrom
codex/secrets-raw-values

Conversation

@shirkevich

@shirkevich Mikhail Shirkov (shirkevich) commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

what

  • Add canonical !secret NAME | raw support, including the compact |raw spelling accepted by the shared function tokenizer.
  • Keep bare !secret structured-by-default and preserve | path for structured lookup; reject raw combined with path.
  • Add optional raw-read capabilities to secret providers and stores so JSON, PEM, numeric, boolean, and quoted payloads can remain byte-for-byte strings when requested.
  • Make string values written by atmos secret set round-trip verbatim instead of storing JSON quote characters.
  • Permit atmos secret set NAME=VALUE --stack ... without --component only when Atmos can prove that NAME has one consistent scope: global declaration.
  • Document the migration behavior for values written by older releases and the JSON type-coercion hazard of bare !secret.
  • Add an offline native Helm regression fixture covering structured JSON, | raw, | path, ordinary strings, multiline keys, masking, and unmasked output.

This is 2 of 2 in a secret-handling follow-up stack:

  1. fix: harden secret validation and masking #2872 — secret declaration validation and serialization-safe masking
  2. Explicit raw secret values and string write/read symmetry

why

Several store backends intentionally JSON-decode values for structured lookup. That is useful for bare !secret and | path, but it turns valid JSON payloads—including service-account documents, numbers, booleans, null, and JSON-quoted strings—into non-string values. Helm charts expecting a scalar can then receive a map or another unexpected type.

The write path had the inverse surprise: atmos secret set JSON-encoded strings, while an explicit raw read faithfully returned the stored quote characters. Values created externally and values created by Atmos therefore required opposite read behavior.

behavior and compatibility

  • Bare !secret retains its existing structured-value contract.
  • | path continues to select structured data.
  • | raw is additive and explicitly requests the original textual payload.
  • New string writes are verbatim. Structured maps and lists supplied through store APIs remain JSON encoded.
  • Values written by older Atmos releases may contain stored JSON quotes and should be written again before switching their references to | raw.
  • Omitting --component remains an error for instance- or stack-scoped declarations, missing declarations, or inconsistent global declarations.

validation

  • 1,720 focused CLI, parser, masker, secret resolver, store-provider, and native Helm tests pass across 11 packages.
  • The Helm fixture uses the in-memory keychain store while reproducing cloud-store JSON decoding; it needs no cloud credentials or network access.
  • Prettier 3.8.4 passes for both changed MDX documents.
  • The two-PR stack reproduces the real-infrastructure integration patch set, plus direct folded-scalar masking coverage.
  • git diff --check and a private-name audit pass.

references

Summary by CodeRabbit

  • New Features
    • Added raw support for retrieving secrets as their original textual values.
    • Added optional path, raw, and default modifiers to !secret.
    • Global secrets can now be set without specifying a component when declarations are consistent.
    • Secret values can be stored verbatim while structured values retain existing behavior.
  • Bug Fixes
    • Improved component validation and clearer errors for ambiguous or incomplete secret targets.
    • Enhanced masking of nested strings in structured secret values.
  • Documentation
    • Updated secret command and !secret syntax documentation with new options and examples.

@atmos-pro

atmos-pro Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Tip

Atmos Pro  

No affected stacks workflow was detected for this pull request.
If this is expected, no action is needed.
Learn More. Ask AI.

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

Dependency Review

✅ No vulnerabilities or license issues found.

Scanned Files

None

@mergify mergify Bot added the stacked Stacked label Aug 5, 2026
@shirkevich
Mikhail Shirkov (shirkevich) marked this pull request as ready for review August 6, 2026 12:14
@shirkevich
Mikhail Shirkov (shirkevich) requested a review from a team as a code owner August 6, 2026 12:14
@github-actions github-actions Bot added size/l Large size PR and removed size/m Medium size PR labels Aug 6, 2026
@shirkevich

Copy link
Copy Markdown
Collaborator Author

CodeRabbit (@coderabbitai) review

@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The change adds component-less global secret setting and introduces raw secret arguments. Raw payload retrieval now spans parsing, resolution, providers, stores, cloud backends, keychain storage, masking, and Helm integration.

Changes

Secret capabilities

Layer / File(s) Summary
Global secret scope resolution
cmd/secret/enumerate.go, cmd/secret/set.go, cmd/secret/shared.go, cmd/secret/set_test.go, cmd/secret/enumerate_test.go, pkg/secrets/scope_test.go, website/docs/cli/commands/secret/set.mdx
Global declarations can resolve without --component when declarations are consistent. Non-global scopes still require a component. Component types remain available during resolution.
Raw secret syntax and resolution
pkg/function/parser/*, pkg/function/secret.go, pkg/secrets/resolver.go, pkg/secrets/types.go, pkg/secrets/resolver_test.go, pkg/io/global.go, website/docs/functions/yaml/secret.mdx
!secret supports path, raw, and default modifiers. Raw resolution returns original textual payloads. JSON objects and arrays are recursively registered for masking.
Raw retrieval contracts and store adapter
pkg/store/store.go, pkg/secrets/providers/provider.go, pkg/secrets/providers/store.go, pkg/secrets/providers/store_test.go
RawStore and RawGetter define raw retrieval. storeProvider.GetRaw uses native raw stores or string-only fallback behavior.
Backend raw storage and retrieval
pkg/store/providers/*
AWS, Azure, Google, GitHub Actions, and Keychain providers add raw retrieval. Secret-aware stores preserve eligible strings during writes and decode values during standard reads.
Helm raw and structured secret validation
tests/fixtures/scenarios/helm-secret-values/*, pkg/component/helm/secret_values_integration_test.go
The Helm fixture and integration test validate structured values, multiline raw secrets, rendered environment variables, and masking behavior.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant YAML
  participant ParseSecret
  participant SecretResolver
  participant Provider
  participant Store
  participant Helm
  YAML->>ParseSecret: parse secret name and modifiers
  ParseSecret->>SecretResolver: provide parsed raw option
  SecretResolver->>Provider: request raw or decoded value
  Provider->>Store: retrieve secret payload
  Store-->>Provider: return payload
  Provider-->>SecretResolver: return resolved value
  SecretResolver-->>Helm: provide chart value
  Helm-->>YAML: render environment variable
Loading

Possibly related PRs

  • cloudposse/atmos#2797: Both changes preserve scalar string values without JSON quoting in secret storage and retrieval.
  • cloudposse/atmos#2858: Both changes modify raw secret retrieval behavior and store-provider support.

Suggested labels: minor

Suggested reviewers: osterman

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the primary change: adding explicit raw secret value support.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/secrets-raw-values

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
cmd/secret/enumerate.go (1)

98-114: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Add ComponentType as a sort tie-breaker.

Line 114 makes ComponentType part of the selected scope. The sort compares only Stack and Component. If two component types use the same component name, map iteration can determine which type findGlobalSetContext selects. The command can then load a different component configuration and write the secret through the wrong context.

Sort by ComponentType after Component, or require --type when this ambiguity exists. Add a regression test with the same component name in two types.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cmd/secret/enumerate.go` around lines 98 - 114, Update the scope-entry
sorting used before findGlobalSetContext to compare ComponentType after Stack
and Component, ensuring entries with identical names have deterministic
ordering. Add a regression test covering identical component names across two
component types and verify the selected context remains consistent.
pkg/secrets/resolver.go (1)

68-88: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Do not use Default for unsupported raw retrieval.

When the provider returns providers.ErrRawNotSupported, Lines 84-87 return opts.Default. The secret can exist, but the requested raw capability is unavailable. This hides a configuration error and can silently use a fallback value.

Apply Default only to missing-secret errors. Preserve ErrRawNotSupported through the wrapped result. Add a resolver test for raw | default with a structured-only provider.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/secrets/resolver.go` around lines 68 - 88, The error handling in the
resolver’s raw retrieval flow must not apply opts.Default when the provider
returns providers.ErrRawNotSupported; restrict the default fallback to
missing-secret errors and preserve ErrRawNotSupported through the existing
wrapped result. Add a resolver test covering raw | default with a
structured-only provider.
🧹 Nitpick comments (3)
cmd/secret/set_test.go (1)

127-194: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use a table-driven test for these lookup scenarios.

The repeated subtests cover one function with the same setup pattern. Put the entries, scope, expected component, and expected error in test cases. This keeps future scope cases consistent.

As per coding guidelines, “Use table-driven tests for testing multiple scenarios in Go.”

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cmd/secret/set_test.go` around lines 127 - 194, Refactor
TestFindGlobalSetContext into a table-driven test covering enumeration errors,
missing matches, inconsistent declarations, and identical declarations. Store
each case’s scope entries, requested scope, secret name, expected
component/type, and expected error in the test table, then run them through a
single subtest loop while preserving the existing assertions and
overrideEnumerateScopes setup.

Source: Coding guidelines

pkg/function/parser/parser_test.go (1)

196-224: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use table-driven cases for parser scenarios.

This test covers multiple input forms with separate assertions. Use one table with input, expected SecretArgs, and expected error fields. Include SERVICE_CONFIG | path "" | raw as an invalid case.

As per coding guidelines, “Use table-driven tests for testing multiple scenarios in Go.”

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/function/parser/parser_test.go` around lines 196 - 224, The
TestParseSecret test should use a single table-driven case list containing each
valid and invalid input, expected SecretArgs, and expected error status/details.
Replace the separate assertions and invalid-input loop while preserving the
existing expectations, and add SERVICE_CONFIG | path "" | raw as an invalid
case.

Source: Coding guidelines

pkg/secrets/providers/provider.go (1)

73-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use the project static error catalog.

ErrRawNotSupported creates a new sentinel directly with errors.New. Define the static error in errors/errors.go, then wrap or expose it from this boundary as needed. Preserve errors.Is behavior.

As per coding guidelines, “Wrap all errors with static errors from errors/errors.go.”

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/secrets/providers/provider.go` around lines 73 - 74, Replace the locally
constructed ErrRawNotSupported sentinel with the corresponding static error
defined in errors/errors.go, then expose or wrap that catalog error from the
provider boundary while preserving errors.Is compatibility for callers.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@cmd/secret/set.go`:
- Around line 99-132: Update findGlobalSetContext to validate each global
declaration’s effective backend coordinate using its component context, rather
than comparing raw declarations alone. Resolve or reject declarations whose
Declaration.Reference depends on atmos_component, and return the existing
componentRequiredForSet error when effective coordinates differ; add coverage
for identical component-dependent declarations.

In `@pkg/function/parser/parser.go`:
- Around line 375-389: Track path-option presence separately from result.Path in
the parser handling around the path and raw cases, so an explicitly empty path
is distinguishable from an omitted path. Update the mutual-exclusion check to
reject raw whenever path was specified, including path "", and add a regression
case for SERVICE_CONFIG | path "" | raw.

In `@pkg/store/providers/google_secret_manager_store.go`:
- Around line 478-484: Add direct unit-test cases for GSMStore.GetRaw covering
both JSON and non-JSON secret payloads. Assert the returned value exactly
matches the backend payload, including its original encoding and content,
without decoding or transformation; retain existing error assertions and use the
established GSM test fixtures and backend mocks.

In `@pkg/store/providers/keychain_store.go`:
- Around line 163-166: Update the JSON string decoding in GetRaw to unmarshal
into a string pointer, returning the decoded value only when the pointer is
non-nil. Preserve the original raw payload for JSON null while retaining the
existing return behavior for non-null strings.

---

Outside diff comments:
In `@cmd/secret/enumerate.go`:
- Around line 98-114: Update the scope-entry sorting used before
findGlobalSetContext to compare ComponentType after Stack and Component,
ensuring entries with identical names have deterministic ordering. Add a
regression test covering identical component names across two component types
and verify the selected context remains consistent.

In `@pkg/secrets/resolver.go`:
- Around line 68-88: The error handling in the resolver’s raw retrieval flow
must not apply opts.Default when the provider returns
providers.ErrRawNotSupported; restrict the default fallback to missing-secret
errors and preserve ErrRawNotSupported through the existing wrapped result. Add
a resolver test covering raw | default with a structured-only provider.

---

Nitpick comments:
In `@cmd/secret/set_test.go`:
- Around line 127-194: Refactor TestFindGlobalSetContext into a table-driven
test covering enumeration errors, missing matches, inconsistent declarations,
and identical declarations. Store each case’s scope entries, requested scope,
secret name, expected component/type, and expected error in the test table, then
run them through a single subtest loop while preserving the existing assertions
and overrideEnumerateScopes setup.

In `@pkg/function/parser/parser_test.go`:
- Around line 196-224: The TestParseSecret test should use a single table-driven
case list containing each valid and invalid input, expected SecretArgs, and
expected error status/details. Replace the separate assertions and invalid-input
loop while preserving the existing expectations, and add SERVICE_CONFIG | path
"" | raw as an invalid case.

In `@pkg/secrets/providers/provider.go`:
- Around line 73-74: Replace the locally constructed ErrRawNotSupported sentinel
with the corresponding static error defined in errors/errors.go, then expose or
wrap that catalog error from the provider boundary while preserving errors.Is
compatibility for callers.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 5ddaa5fb-4e7d-4fa4-be16-447fd3de79dc

📥 Commits

Reviewing files that changed from the base of the PR and between 40acdd3 and ab4f2fd.

📒 Files selected for processing (32)
  • cmd/secret/enumerate.go
  • cmd/secret/set.go
  • cmd/secret/set_test.go
  • cmd/secret/shared.go
  • pkg/component/helm/secret_values_integration_test.go
  • pkg/function/parser/parser.go
  • pkg/function/parser/parser_test.go
  • pkg/function/secret.go
  • pkg/io/global.go
  • pkg/secrets/providers/provider.go
  • pkg/secrets/providers/store.go
  • pkg/secrets/providers/store_test.go
  • pkg/secrets/resolver.go
  • pkg/secrets/resolver_test.go
  • pkg/secrets/types.go
  • pkg/store/providers/aws_secrets_manager_store.go
  • pkg/store/providers/aws_secrets_manager_store_endpoint_test.go
  • pkg/store/providers/aws_ssm_param_store.go
  • pkg/store/providers/azure_keyvault_store.go
  • pkg/store/providers/azure_keyvault_store_test.go
  • pkg/store/providers/github_actions_store.go
  • pkg/store/providers/google_secret_manager_store.go
  • pkg/store/providers/google_secret_manager_store_test.go
  • pkg/store/providers/keychain_store.go
  • pkg/store/providers/keychain_store_test.go
  • pkg/store/store.go
  • tests/fixtures/scenarios/helm-secret-values/atmos.yaml
  • tests/fixtures/scenarios/helm-secret-values/components/helm/secret-values/Chart.yaml
  • tests/fixtures/scenarios/helm-secret-values/components/helm/secret-values/templates/deployment.yaml
  • tests/fixtures/scenarios/helm-secret-values/stacks/deploy/dev.yaml
  • website/docs/cli/commands/secret/set.mdx
  • website/docs/functions/yaml/secret.mdx

Comment thread cmd/secret/set.go
Comment thread pkg/function/parser/parser.go
Comment thread pkg/store/providers/google_secret_manager_store.go
Comment thread pkg/store/providers/keychain_store.go Outdated
@shirkevich

Copy link
Copy Markdown
Collaborator Author

CodeRabbit (@coderabbitai) review\n\nPlease review current head 5eba834e65ee4a0086e144baa9a1681196cd4c9b, which addresses the prior actionable findings.

@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

@shirkevich I will perform a full review of current head 5eba834e65ee4a0086e144baa9a1681196cd4c9b, including the changes that address the prior findings.

✅ Action performed

Full review finished.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

🧹 Nitpick comments (3)
cmd/secret/set_test.go (1)

127-194: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use a table-driven test for the context-resolution cases.

The four subtests repeat the setup, invocation, and error assertions. Put the cases in a table with expected component, type, and error state.

As per coding guidelines, “Use table-driven tests for testing multiple scenarios in Go.”

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cmd/secret/set_test.go` around lines 127 - 194, Refactor
TestFindGlobalSetContext into a table-driven test covering the four existing
context-resolution scenarios. Define each case with its scope entries,
enumeration error, expected component and component type, and whether an error
is expected; iterate with subtests, applying overrideEnumerateScopes and
invoking findGlobalSetContext once per case while preserving the existing
assertions and outcomes.

Source: Coding guidelines

pkg/function/parser/parser_test.go (1)

196-225: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use table-driven tests for parser scenarios.

Both tests exercise multiple independent parser scenarios. Put each input and expected result or error assertion in a test table.

  • pkg/function/parser/parser_test.go#L196-L225: table-drive valid and invalid ParseSecret inputs.
  • pkg/secrets/resolver_test.go#L181-L196: table-drive raw, default, compact syntax, conflict, and empty-name cases.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/function/parser/parser_test.go` around lines 196 - 225, Convert
TestParseSecret in pkg/function/parser/parser_test.go (lines 196-225) into
table-driven cases covering each valid input with its expected SecretArgs or
field assertions, plus invalid inputs expecting errors. Also convert the raw,
default, compact syntax, conflict, and empty-name scenarios in
pkg/secrets/resolver_test.go (lines 181-196) into a table-driven test,
preserving each scenario’s existing expectations.

Source: Coding guidelines

pkg/secrets/resolver_test.go (1)

154-173: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a raw missing-secret fallback test.

This test confirms that ErrRawNotSupported does not use the default. Add a case where raw retrieval returns a missing-secret error and assert that raw | default "fallback" returns "fallback".

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/secrets/resolver_test.go` around lines 154 - 173, Add a test alongside
TestResolve_RawDefaultDoesNotHideUnsupportedCapability that configures the mock
store to return the missing-secret error for DATADOG_API_KEY, then resolves the
raw secret expression with default "fallback" and asserts it returns "fallback"
without an error. Keep the existing unsupported-capability test unchanged.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@cmd/secret/set_test.go`:
- Around line 122-125: Update the test around runSecretSubcommand and its
loadServiceFn mock to capture the secretScope argument passed to the loader,
then assert that its ComponentType is "helm" in addition to the existing
call-count checks. Keep the assertion behavior-focused and table-driven if the
surrounding tests support that pattern.

In `@pkg/component/helm/secret_values_integration_test.go`:
- Line 52: Refactor
TestNativeHelmSecretRawAndStructuredValuesMaskIndentedMultilineValues into named
helpers so it stays under 60 lines and separates rendering from masking
assertions. In the fallback manifest path, use require.NotEmpty before accessing
containers[0], then validate manifest and container types with require.IsType
before asserting and reading env; ensure malformed fixtures fail through test
assertions rather than panicking.

In `@pkg/function/parser/parser.go`:
- Line 340: Correct the grammar comment for ParseSecret to show that path
expression and raw are alternatives rather than independently optional, matching
the validation that rejects using both together in the parser logic.

In `@pkg/secrets/providers/store_test.go`:
- Around line 106-129: Add a test alongside
TestStoreProvider_GetRawFallsBackForTextOnly using a mock store that implements
store.RawStore. Configure the raw-store method to return a raw payload, assert
GetRaw returns it without error, and do not set any Store.Get expectation so the
test verifies native RawStore delegation bypasses the fallback.

In `@pkg/store/providers/aws_secrets_manager_store.go`:
- Around line 296-311: Add repository-standard deferred perf.Track
instrumentation, using each method’s atmosConfig and fully qualified function
name, at the start of SecretsManagerStore.GetRaw in
pkg/store/providers/aws_secrets_manager_store.go:296-311, GSMStore.GetRaw in
pkg/store/providers/google_secret_manager_store.go:478-484, and
KeychainStore.GetRaw in pkg/store/providers/keychain_store.go:151-168; include
the required blank line after each instrumentation statement.

In `@pkg/store/providers/aws_ssm_param_store.go`:
- Around line 426-435: Update the inline comments above getKey and assumeRole in
the parameter-name retrieval flow so each comment ends with a period, without
changing the surrounding logic.

In `@pkg/store/providers/azure_keyvault_store.go`:
- Around line 369-389: Update AzureKeyVaultStore.GetRaw and Set to allow empty
stack and/or component values so getKey can resolve stack-scoped and global
coordinates consistently with GSMStore; retain key validation and existing error
handling. Ensure !secret NAME | raw works and component-less global writes are
supported, then add coverage for stack-scoped and global reads and writes.

---

Nitpick comments:
In `@cmd/secret/set_test.go`:
- Around line 127-194: Refactor TestFindGlobalSetContext into a table-driven
test covering the four existing context-resolution scenarios. Define each case
with its scope entries, enumeration error, expected component and component
type, and whether an error is expected; iterate with subtests, applying
overrideEnumerateScopes and invoking findGlobalSetContext once per case while
preserving the existing assertions and outcomes.

In `@pkg/function/parser/parser_test.go`:
- Around line 196-225: Convert TestParseSecret in
pkg/function/parser/parser_test.go (lines 196-225) into table-driven cases
covering each valid input with its expected SecretArgs or field assertions, plus
invalid inputs expecting errors. Also convert the raw, default, compact syntax,
conflict, and empty-name scenarios in pkg/secrets/resolver_test.go (lines
181-196) into a table-driven test, preserving each scenario’s existing
expectations.

In `@pkg/secrets/resolver_test.go`:
- Around line 154-173: Add a test alongside
TestResolve_RawDefaultDoesNotHideUnsupportedCapability that configures the mock
store to return the missing-secret error for DATADOG_API_KEY, then resolves the
raw secret expression with default "fallback" and asserts it returns "fallback"
without an error. Keep the existing unsupported-capability test unchanged.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 0d4e8a9a-25fa-4fc2-8cc8-21c66d6cb493

📥 Commits

Reviewing files that changed from the base of the PR and between 40acdd3 and 5eba834.

📒 Files selected for processing (34)
  • cmd/secret/enumerate.go
  • cmd/secret/enumerate_test.go
  • cmd/secret/set.go
  • cmd/secret/set_test.go
  • cmd/secret/shared.go
  • pkg/component/helm/secret_values_integration_test.go
  • pkg/function/parser/parser.go
  • pkg/function/parser/parser_test.go
  • pkg/function/secret.go
  • pkg/io/global.go
  • pkg/secrets/providers/provider.go
  • pkg/secrets/providers/store.go
  • pkg/secrets/providers/store_test.go
  • pkg/secrets/resolver.go
  • pkg/secrets/resolver_test.go
  • pkg/secrets/scope_test.go
  • pkg/secrets/types.go
  • pkg/store/providers/aws_secrets_manager_store.go
  • pkg/store/providers/aws_secrets_manager_store_endpoint_test.go
  • pkg/store/providers/aws_ssm_param_store.go
  • pkg/store/providers/azure_keyvault_store.go
  • pkg/store/providers/azure_keyvault_store_test.go
  • pkg/store/providers/github_actions_store.go
  • pkg/store/providers/google_secret_manager_store.go
  • pkg/store/providers/google_secret_manager_store_test.go
  • pkg/store/providers/keychain_store.go
  • pkg/store/providers/keychain_store_test.go
  • pkg/store/store.go
  • tests/fixtures/scenarios/helm-secret-values/atmos.yaml
  • tests/fixtures/scenarios/helm-secret-values/components/helm/secret-values/Chart.yaml
  • tests/fixtures/scenarios/helm-secret-values/components/helm/secret-values/templates/deployment.yaml
  • tests/fixtures/scenarios/helm-secret-values/stacks/deploy/dev.yaml
  • website/docs/cli/commands/secret/set.mdx
  • website/docs/functions/yaml/secret.mdx

Comment thread cmd/secret/set_test.go
Comment on lines +122 to +125
err := runSecretSubcommand(t, "set", "SHARED_TOKEN=v1", "--stack", "dev", "--type", "helm")
require.NoError(t, err)
require.Len(t, svc.setCalls, 1)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Assert the resolved component type.

This test passes if parseSetScope drops or overwrites --type. svc.Set does not expose the scope used by loadServiceFn.

Capture the secretScope passed to loadServiceFn, then assert that ComponentType is "helm".

As per coding guidelines, “Prefer behavior-focused, table-driven unit tests with mocks.”

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cmd/secret/set_test.go` around lines 122 - 125, Update the test around
runSecretSubcommand and its loadServiceFn mock to capture the secretScope
argument passed to the loader, then assert that its ComponentType is "helm" in
addition to the existing call-count checks. Keep the assertion behavior-focused
and table-driven if the surrounding tests support that pattern.

Source: Coding guidelines

return s.Store.(store.RawStore).GetRaw(stack, component, key)
}

func TestNativeHelmSecretRawAndStructuredValuesMaskIndentedMultilineValues(t *testing.T) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Split the test and validate manifest types before access.

The test exceeds the 60-line limit. The fallback also indexes containers[0] and uses unchecked type assertions. A malformed manifest will panic instead of reporting the failed fixture contract.

Extract rendering and masking assertions into helpers. Use require.NotEmpty and type assertions guarded by require.IsType before accessing env.

As per coding guidelines, “Safety precondition and fixture-count checks must fail loudly” and “Refactor functions exceeding ... 60 lines/40 statements into named, single-responsibility helpers.”

Also applies to: 106-113

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/component/helm/secret_values_integration_test.go` at line 52, Refactor
TestNativeHelmSecretRawAndStructuredValuesMaskIndentedMultilineValues into named
helpers so it stays under 60 lines and separates rendering from masking
assertions. In the fallback manifest path, use require.NotEmpty before accessing
containers[0], then validate manifest and container types with require.IsType
before asserting and reading env; ensure malformed fixtures fail through test
assertions rather than panicking.

Source: Coding guidelines

Comment thread pkg/function/parser/parser.go Outdated
return StoreGetArgs{Store: words[0], Key: words[1], Default: options.defaultValue, Query: options.query}, nil
}

// ParseSecret parses `name [| path expression] [| raw] [| default value]`.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Correct the ParseSecret grammar comment.

Line 340 presents path and raw as independently optional. Lines 390-391 reject their combination. Show them as alternatives.

Proposed fix
-// ParseSecret parses `name [| path expression] [| raw] [| default value]`.
+// ParseSecret parses `name [| path expression | raw] [| default value]`.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// ParseSecret parses `name [| path expression] [| raw] [| default value]`.
// ParseSecret parses `name [| path expression | raw] [| default value]`.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/function/parser/parser.go` at line 340, Correct the grammar comment for
ParseSecret to show that path expression and raw are alternatives rather than
independently optional, matching the validation that rejects using both together
in the parser logic.

Source: Coding guidelines

Comment on lines +106 to +129
func TestStoreProvider_GetRawFallsBackForTextOnly(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()

mockStore := store.NewMockStore(ctrl)
mockStore.EXPECT().Get("prod", "api", "API_KEY").Return("v1", nil)
p := &storeProvider{name: "app", kind: "example/text", store: mockStore}

got, err := p.GetRaw(Coordinate{Stack: "prod", Component: "api", Key: "API_KEY"})
require.NoError(t, err)
assert.Equal(t, "v1", got)
}

func TestStoreProvider_GetRawRejectsStructuredFallback(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()

mockStore := store.NewMockStore(ctrl)
mockStore.EXPECT().Get("prod", "api", "CONFIG").Return(map[string]any{"enabled": true}, nil)
p := &storeProvider{name: "app", kind: "example/structured", store: mockStore}

_, err := p.GetRaw(Coordinate{Stack: "prod", Component: "api", Key: "CONFIG"})
require.ErrorIs(t, err, ErrRawNotSupported)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Add coverage for native store.RawStore delegation.

The tests cover only the fallback branch. Add a test where p.store implements store.RawStore. Assert that GetRaw returns the raw payload and does not call Store.Get.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/secrets/providers/store_test.go` around lines 106 - 129, Add a test
alongside TestStoreProvider_GetRawFallsBackForTextOnly using a mock store that
implements store.RawStore. Configure the raw-store method to return a raw
payload, assert GetRaw returns it without error, and do not set any Store.Get
expectation so the test verifies native RawStore delegation bypasses the
fallback.

Source: Coding guidelines

Comment on lines +296 to +311
// GetRaw retrieves the original Secrets Manager string without JSON decoding.
func (s *SecretsManagerStore) GetRaw(stack string, component string, key string) (string, error) {
if key == "" {
return nil, store.ErrEmptyKey
return "", store.ErrEmptyKey
}

if err := s.ensureClient(); err != nil {
return nil, err
return "", err
}

secretID, err := s.getKey(stack, component, key)
if err != nil {
return nil, fmt.Errorf(errWrapFormat, store.ErrGetKey, err)
return "", fmt.Errorf(errWrapFormat, store.ErrGetKey, err)
}

return s.getByID(secretID)
return s.getRawByID(secretID)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Instrument all new GetRaw methods.

Each new public raw-read operation lacks the required performance tracker.

  • pkg/store/providers/aws_secrets_manager_store.go#L296-L311: Add repository-standard perf.Track instrumentation to SecretsManagerStore.GetRaw.
  • pkg/store/providers/google_secret_manager_store.go#L478-L484: Add repository-standard perf.Track instrumentation to GSMStore.GetRaw.
  • pkg/store/providers/keychain_store.go#L151-L168: Add repository-standard perf.Track instrumentation to KeychainStore.GetRaw.

As per coding guidelines, “Add defer perf.Track(atmosConfig, "pkg.FuncName")() plus a blank line to public functions.”

📍 Affects 3 files
  • pkg/store/providers/aws_secrets_manager_store.go#L296-L311 (this comment)
  • pkg/store/providers/google_secret_manager_store.go#L478-L484
  • pkg/store/providers/keychain_store.go#L151-L168
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/store/providers/aws_secrets_manager_store.go` around lines 296 - 311, Add
repository-standard deferred perf.Track instrumentation, using each method’s
atmosConfig and fully qualified function name, at the start of
SecretsManagerStore.GetRaw in
pkg/store/providers/aws_secrets_manager_store.go:296-311, GSMStore.GetRaw in
pkg/store/providers/google_secret_manager_store.go:478-484, and
KeychainStore.GetRaw in pkg/store/providers/keychain_store.go:151-168; include
the required blank line after each instrumentation statement.

Source: Coding guidelines

Comment on lines 426 to +435
// Construct the full parameter name using getKey
paramName, err := s.getKey(stack, component, key)
if err != nil {
return nil, fmt.Errorf(errWrapFormat, store.ErrGetKey, err)
return "", fmt.Errorf(errWrapFormat, store.ErrGetKey, err)
}

// Assume the read role if specified
cfg, err := s.assumeRole(ctx, s.readRoleArn)
if err != nil {
return nil, fmt.Errorf(errWrapFormat, store.ErrAssumeRole, err)
return "", fmt.Errorf(errWrapFormat, store.ErrAssumeRole, err)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Terminate the inline comments with periods.

Line 426 and Line 432 add comments without final periods. This violates the Go comment rule.

Proposed fix
-	// Construct the full parameter name using getKey
+	// Construct the full parameter name using getKey.
@@
-	// Assume the read role if specified
+	// Assume the read role if specified.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Construct the full parameter name using getKey
paramName, err := s.getKey(stack, component, key)
if err != nil {
return nil, fmt.Errorf(errWrapFormat, store.ErrGetKey, err)
return "", fmt.Errorf(errWrapFormat, store.ErrGetKey, err)
}
// Assume the read role if specified
cfg, err := s.assumeRole(ctx, s.readRoleArn)
if err != nil {
return nil, fmt.Errorf(errWrapFormat, store.ErrAssumeRole, err)
return "", fmt.Errorf(errWrapFormat, store.ErrAssumeRole, err)
// Construct the full parameter name using getKey.
paramName, err := s.getKey(stack, component, key)
if err != nil {
return "", fmt.Errorf(errWrapFormat, store.ErrGetKey, err)
}
// Assume the read role if specified.
cfg, err := s.assumeRole(ctx, s.readRoleArn)
if err != nil {
return "", fmt.Errorf(errWrapFormat, store.ErrAssumeRole, err)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/store/providers/aws_ssm_param_store.go` around lines 426 - 435, Update
the inline comments above getKey and assumeRole in the parameter-name retrieval
flow so each comment ends with a period, without changing the surrounding logic.

Source: Coding guidelines

Comment on lines +369 to +389
func (s *AzureKeyVaultStore) GetRaw(stack string, component string, key string) (string, error) {
if stack == "" {
return nil, store.ErrEmptyStack
return "", store.ErrEmptyStack
}
if component == "" {
return nil, store.ErrEmptyComponent
return "", store.ErrEmptyComponent
}
if key == "" {
return nil, store.ErrEmptyKey
return "", store.ErrEmptyKey
}

if err := s.ensureClient(); err != nil {
return nil, err
return "", err
}

secretName, err := s.getKey(stack, component, key)
if err != nil {
return nil, fmt.Errorf(errWrapFormat, store.ErrGetKey, err)
return "", fmt.Errorf(errWrapFormat, store.ErrGetKey, err)
}
return s.getRawByName(secretName)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Support stack-scoped and global secret coordinates.

GetRaw rejects an empty stack or component before getKey can resolve the coordinate. This makes !secret NAME | raw fail for stack-scoped and global secrets. Set has the same validation at lines 293-298, so global writes also fail for Azure Key Vault.

Allow omitted scope segments consistently with GSMStore, then add stack-scoped and global read/write coverage.

The PR objective states that component-less global writes are supported.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/store/providers/azure_keyvault_store.go` around lines 369 - 389, Update
AzureKeyVaultStore.GetRaw and Set to allow empty stack and/or component values
so getKey can resolve stack-scoped and global coordinates consistently with
GSMStore; retain key validation and existing error handling. Ensure !secret NAME
| raw works and component-less global writes are supported, then add coverage
for stack-scoped and global reads and writes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/l Large size PR stacked Stacked

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant